2단계 - 로또(자동) 리뷰 요청드립니다. #1080
2단계 - 로또(자동) 리뷰 요청드립니다. #1080
Conversation
src/main/kotlin/lotto/README.md
Outdated
| - [ ] lotto List를 전달받는다. | ||
| - [ ] 내부 lotto List의 기능을 Lottos가 위임받는다. |
There was a problem hiding this comment.
기능 구현 목록표는 객체의 역할 혹은 책임에 관점에서 적어주시면 좋을 것 같아요.
어떻게 구현할 것인가 어떤 상태를 가질 것인가 이런 내용을 적는 것이 조금 더 좋은 기능 구현 목록표가 될 것 같아요. 😃
지금 같이 구체적인 구현단의 내용을 문서로 정리하게되면, 세부적인 구현 내용이 바뀌게된 경우에도 일일이 문서도 수정해야하는 번거로움이 따를 수 있고, 문서가 업데이트되지 않는 순간부터는 이 문서는 죽은 문서가 될 것 같아요. 🥲
There was a problem hiding this comment.
입력값과 반환값은 제외하고, 기능만을 기술하도록 문서를 수정하였습니다.
| val lottos = LottoMarketService.start(purchaseAmount) | ||
| ResultView.printLottos(lottos) | ||
|
|
||
| val winningNumber = InputView.inputWinningNumber() |
There was a problem hiding this comment.
당첨 번호는 6개의 로또 번호라고 느껴지는데요. 그렇다면 네이밍적으로도 복수형인 inputWinningNumbers가 조금 더 직관적으로 결과물을 이해할 수 있지 않을까요?
There was a problem hiding this comment.
네 그게 좋을거 같습니다.
의견 주신 이름으로 함수명을 수정하겠습니다.
|
|
||
| import lotto.core.constant.LottoConstants | ||
|
|
||
| data class WinningNumbers(val winningNumbers: List<Int>) { |
There was a problem hiding this comment.
당첨번호들은 6개의 로또 번호를 가지는군요?
이미 구현해두신 Lotto와 비슷하게 느껴지시지 않으시나요? 😃
WinningNumbers를 따로 만들 필요 없이, 당첨 번호 == 로또 라고 볼 수도 있지 않을까요? 🤔
There was a problem hiding this comment.
네 만들고 난 다음에는 말씀하신것과 같이 기능적으로 Lotto와 같다고 생각했었습니다만,
로또는 발급의 대상으로 상품이라 인지하고, 당첨번호는 로또의 상태를 변화시키는 조건 같은 것이라 둘의 성질은 다른것이라고 생각했습니다.
또, 이후 보너스 넘버와 같은 추가 조건이 들어올 수 있다고 생각도 되었고, 그 경우 WinningNumbers를 Lotto와 같은 것으로 만들었을 때는 멤버가 달라지게 되서, 오히려 속성이 멀어지게 될거 같다는 생각이 듭니다.
물론 상속을 받아서 구현하는 방법도 있긴 합니다만, 그렇게 되면 생성자때문에 Data Class로는 만들기 어렵지 않을까 생각도 되고요.
There was a problem hiding this comment.
네 만들고 난 다음에는 말씀하신것과 같이 기능적으로 Lotto와 같다고 생각했었습니다만,
로또는 발급의 대상으로 상품이라 인지하고, 당첨번호는 로또의 상태를 변화시키는 조건 같은 것이라 둘의 성질은 다른것이라고 생각했습니다.
말씀하신 것처럼 자칫하면 어설픈 추상화가 될 수도 있지만 기능적인 것 뿐만이아니라 비즈니스 규칙조차 (1~45 범위의 로또 숫자를 6개만 가진다)도 완벽히 일치하기 때문에 저는 동일한 객체로 볼 수 있다고 생각해요. 😃
말씀하신 것처럼 추후 보너스 넘버와 같은 추가 조건이 생길 수도 있을거고 말씀하신 상속의 방식도 하나의 방법이 될 수도 있습니다만 단지 같은 상태를 가지게하기위해 혹은 중복을 제거하기위해 상속을 사용하는 것은 그렇게 권장되는 방식은 아닙니다. 😃
상속이 is a의 관계를 가진다면 has a 관계를 가지는 조합의 개념도 있는데요.
아래의 글을 참고해보시고 이러한 상황이 필요하다면 조합의 방식도 고려해보시면 어떨까 싶어요. 😃
상속보다는 조합(Composition)을 사용하자.
There was a problem hiding this comment.
네. 말씀하신부분에 대해서 이해했습니다.
조합에 관련된 부분은 WinningNumbers Class는 Step3에서 보너스 넘버를 구현하면서 조합으로 변경해볼까 합니다.
|
|
||
| object LottoMarketService { | ||
| fun start(purchaseAmount: String?): Lottos { | ||
| return LottoMarket.purchase(purchaseAmount) |
There was a problem hiding this comment.
LottoMarket 객체는 로또를 구매하는 역할을 수행하는 것 같은데요.
LottoMarketService 라는 서비스 객체를 만드신 이유가 있을까요? 😃
LottoMarketService.start 대신에 LottoMarket.purchase를 바로 호출해도 될 것 같은데 찬희님은 어떻게 생가하시나요?
There was a problem hiding this comment.
LottoMarket은 Domain으로 생각을 했고, LottoMarketService는 서비스라고 생각을 했습니다.
LottoMarket은 로또를 발급하는 역할을 책임지는 주체이고, LottoMarketService는 로또를 발급하는 행위를 진행하는 과정이라고 생각했습니다.
제가 도메인과 서비스의 개념을 최근에 접해서 이해도가 좀 낮습니다.
이런 식으로 분리하는 것이 아닌가요? ^^;;;
의견 부탁드립니다.
There was a problem hiding this comment.
객체지향에서 객체가 자신의 역할을 수행하기위해 스스로 행위를하는데요.
지금과 같은 경우에는 거의 모든 도메인 모델에 대해서 1:1로 서비스 객체가 생성되어야할 것 같은데요.
서비스 객체 없이도 객체가 스스로 행동하도록 수정한다면 모든 (혹은 대부분의) 서비스 로직은 제거해도 무방할 것 같아요.
이와는 별개로 서비스 개념이 익숙하지 않으시다면 잠시 내려놓으셔도 좋을 것 같습니다. 😃
실제로 조금 복잡한 레벨에 들어서야 서비스 계층이 필요해지기 때문에 지금과 같이 간단한 미션들을 구현함에 있어서는 사실 서비스 레이어가 꼭 필요하지 않다고 생각해요. 🤔
There was a problem hiding this comment.
네 의견 감사합니다.
조금 다시 살펴보니 LottoMarket이 이미 서비스 적인 성향을 띄고 있다는 생각이 드네요.
역할 분배가 잘못되었던가, 아니면 LottoMarket이 도메인이 아닌 서비스인데 제가 잘못 분류하고 있는게 아닌가 생각됩니다.
| package lotto.presentation | ||
|
|
||
| object InputView { | ||
| fun inputPurchaseAmount(): String? { |
There was a problem hiding this comment.
입력을 받을 때 가급적 non-nullable type으로 입력을 받는 것은 어떨까요?
null에 대한 예외 처리는 View 단에서도 충분히해줄 수 있다고 생각해요. 😃
| lottos.forEach { | ||
| it.checkWinningStates(winningNumbers) | ||
| } |
There was a problem hiding this comment.
WinningChecker 에게 당첨 여부를 판단하는 역할을 위임한 것 같은데요. 🤔
이 역할을 lottos 객채에게 위임해보면 어떨까요?
lottos.matchWinningRank(winningNumbers)
src/main/kotlin/lotto/core/Lottos.kt
Outdated
| @@ -0,0 +1,3 @@ | |||
| package lotto.core | |||
|
|
|||
| class Lottos(private val lottoList: List<Lotto>) : List<Lotto> by lottoList | |||
There was a problem hiding this comment.
by 키워드를 활용한 위임 👍💯
물론 적절한 활용일 수도 있지만 한편으로는 불필요한 List의 public 메서드들을 정의한 것이기 때문에 캡슐화가 깨지는 것처럼 보이는 것 같은데요. 찬희님은 by 키워드를 활용하신 이유가 무엇인가요? 😃
There was a problem hiding this comment.
캡슐화를 생각하진 않고, 한번도 안써본거라 그냥 써보고 싶었던게 가장 큽니다. ^^;;;
(코틀린에는 별게 다 있네요.)
리스트를 List가 아닌 하나의 Class Name으로 치환해서 사용하는게 재미있어보였습니다.
There was a problem hiding this comment.
한번도 안써본거라 써보고 싶었다고 하시니 캡슐화 적인 얘기는 이번에 참고만해주시고 이번엔 by 키워드를 활용해보시는 걸로하시죠! 😃
| companion object { | ||
| private fun splitNumbers(winningNumbers: String?): List<Int> { | ||
| if (winningNumbers == null) { | ||
| throw RuntimeException() |
There was a problem hiding this comment.
RuntimeException보다는 더 구체적인 표준 예외들을 적절히 사용해주시면 좋을 것 같습니다. 😃
추가적으로 예외 케이스를 쉽게 디버깅하기위해 적절한 예외 메시지도 꼭 추가해주셨으면합니다. 😉
There was a problem hiding this comment.
예외 메시지를 추가하고, Exception의 Type을 변경하엿습니다.
| constructor(winningNumbers: String?) : this(splitNumbers(winningNumbers)) | ||
|
|
||
| companion object { | ||
| private fun splitNumbers(winningNumbers: String?): List<Int> { |
There was a problem hiding this comment.
입력에 대한 검증 처리는 View에서 처리해주는 것이 맞다고 생각해요. 😃
지금 현재의 경우에는 Model이 View의 책임까지 수행하고있는 것으로 생각돼요.
적절히 입력에 대한 검증들은 InputView에 위임해보면 어떨까요? 😃
| if (numberList.size < LottoConstants.LOTTO_NUMBER_COUNT) { | ||
| throw RuntimeException() | ||
| } |
There was a problem hiding this comment.
꼭 지금과 같은 문자열 입력 값 뿐만아니라 애초에 List 타입의 winningNumbers 를 생성자 파라미터로 주입받더라도 여섯 자리의 로또 숫자라는 비즈니스 규칙은 변하지 않는데요.
이런 유효성 검증은 init 블록에서 WinningNumbers의 유효성 검증을 수행하도록해보면 어떨까요?
1. InputView의 리턴값을 Non-Nullable로 수정 2. 관련 Method와 Class의 인자를 Non-Nullable로 타입 변경.
문자열에서 숫자 분리하는 기능을 InputView로 이관
|
안녕하세요 리뷰어님. |
| val winningNumbers = WinningNumbers(numbers) | ||
| val winningRankCount = lottos.countWinningRanks(winningNumbers) | ||
|
|
||
| return LottoWinningStatistics(winningRankCount, YieldCalculator.calculate(winningRankCount, lottos.size)) |
There was a problem hiding this comment.
LottoWinningStatisticsService 서비스 객체도 적절히 역할을 잘 위임해준다면 도메인 객체로도 충분히 해결할 수 있을 것 같아요. 😃
lottos.countWinningRanks(winningNumbers)의 결과가 단순히
winningRankCount가 현재는 Map<WinningRank, Int>의 결과로 반환되고있지만 (이 값을 상태 값으로 가진) LottoResult 같은 객체에게 역할을 위임한다면 이 LottoResult 객체에게 (YieldCalculator가 수행하고있는) 이익률을 계산하는 역할을 위임해볼 수 있을 것 같아요.
| package lotto.core | ||
|
|
||
| enum class WinningRank(val winningCount: Int, val winningAmount: Int) { | ||
| RANK0(0, 0), |
There was a problem hiding this comment.
0은 꽝이고 1은 1등의 의미일까요...?
조금 헷갈리는 넘버링인 것 같아요. 🤔
또한 enum class의 네이밍에 이미 Rank가 들어가기 때문에 RANK라는 prefix가 없어도 동일한
ex) WinningRank.FIRST
| ; | ||
|
|
||
| companion object { | ||
| private val map = entries.associateBy(WinningRank::winningCount) |
There was a problem hiding this comment.
이 map은 상수라고 생각되는데요. 상수 네이밍 규칙을 따라주시면 좋을 것 같아요. 😃
또한 조금 더 네이밍도 의미있게 변경해주셨으면합니다. 🙏
|
|
||
| class WinningRankTest { | ||
| @Test | ||
| fun `WinningCount로 WinningRank를 정상적으로 얻어지는 것을 확인한다`() { |
There was a problem hiding this comment.
이미 @CsvSource, @ValueSource 등을 사용해서 여러가지 파라미터에 대한 테스트를 잘 작성해주셨네요!
이 테스트 또한 하나의 테스트 케이스에 여러 검증을 한번에 하기보다는 JUnit의 Parameterized Test를 활용해서 해당 테스트 코드를 조금 더 까끔하게 변경해보시면 좋을 것 같아요. 😃
| class YieldCalculatorTest { | ||
| @Test | ||
| fun `수익률 계산을 검증한다`() { | ||
| YieldCalculator.calculate(mapOf(WinningRank.RANK4 to 2, WinningRank.RANK3 to 1), 5) shouldBe 12 |
There was a problem hiding this comment.
테스트 코드도 유지보수의 대상이기 때문에 테스트 코드 또한 어느정도는 읽기 좋은 코드로 유지해야한다고 생각해요.
꼭 이 양식을 따르라는건 아니지만 테스트 케이스는 given, when, then이 잘 드러나도록 해주시면
추후 유지보수에도 도움이 될거에요. 😁
| YieldCalculator.calculate(mapOf(WinningRank.RANK4 to 2, WinningRank.RANK3 to 1), 5) shouldBe 12 | |
| val winningRankCount = mapOf(WinningRank.RANK4 to 2, WinningRank.RANK3 to 1) | |
| val lottoCount = 5 | |
| val actual = YieldCalculator.calculate(winningRankCount, lottoCount) | |
| actual shouldBe 12 |
안녕하세요 리뷰어님.
조금 늦었습니다만, 리뷰 부탁드립니다.
작성하면서 고민했던 부분이, 너무 세부적인 기능으로 나뉘어 Domain Class가 너무 많이 생긴게 아닌가 싶은데요.
관련해서 의견도 부탁드립니다.
좋은 하루 되십시오.
감사합니다.